goeun - edges - hotel by goeunpark · Pull Request #22 · Ada-C10/hotel

goeunpark · 2018-09-10T06:40:42Z

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question	Answer
Describe a design decision you had to make when working on this project. What options were you considering? What helped you make your final decision?	I initially over-designed and my classes had too much dependency (rooms knew which reservations they were associated with) and I thought I would create a lot more data structures than necessary (a whole array in `HotelBooker` for all the dates ever booked). Thinking about how Chapter 3 Sandi Metz would be unimpressed with my agenda helped. Having a looming deadline made me feel rushed so I went with options that felt like it would let me get away with bare minimum. In retrospect, that was a pretty good design plan.
Describe a concept that you gained more clarity on as you worked on this assignment.	`HotelBooker` is my factory class. Initially, I had `Reservation`s create `Room`s and `Room`s refer to `DateRange`s but then it got too messy so I put all the logic into my main class. I see now that `TripDispatcher` in Rideshare was set up to instantiate all the objects for a reason!
Describe a nominal test that you wrote for this assignment.	I created one reservation for one date range (lines 32 - 41 in `HotelBooker`). Easy. Things seemed to work.
Describe an edge case test that you wrote for this assignment.	I created 20 reservations for one date range (lines 43 - 49 in `HotelBooker`). Then I tried to create one more. But there were no more available rooms, so I raised an `StandardError`
How do you feel you did in writing pseudocode first, then writing the tests and then the code?	Feel pretty good about writing pseudocode! I feel like I was A LOT better at git hygiene and TDD with a partner. Will work on it

…d room

…, and can get total_cost of reservation

…t; test for find_reservations(date) in hotel_booker is red

tildeee · 2018-09-21T06:11:28Z

Hotel

What We're Looking For

Feature	Feedback
Baseline
Used git regularly	x
Answer comprehension questions	x
Design
Each class is responsible for a single piece of the program	x
Classes are loosely coupled	x
Wave 1
List rooms	x
Reserve a room for a given date range	x
List reservations for a given date	x
Calculate reservation price	x
Invalid date range produces an error	x
Test coverage	x
Wave 2
View available rooms for a given date range	x
Reserving a room that is not available produces an error	x
Test coverage	x
Wave 3
Create a block of rooms	x
Check if a block has rooms	x
Reserve a room from a block	x
Test coverage	x
Fundamentals
Names variables, classes and modules appropriately	x, fantastic!
Understanding of variable scope - local vs instance	x
Can create complex logical structures utilizing variables	x
Appropriately uses methods to break down tasks into smaller simpler tasks	x
Understands the differences between class and instance methods	x
Appropriately uses iterators and enumerables	x
Appropriately writes and utilizes classes	x
Appropriately utilizes modules as a mechanism to encapsulate functionality	x
Wrap Up
There is a refactors.txt file	x
The file provides a roadmap to future changes	x
Additional Feedback

Goeun! Good job with this project!

I think that your code looks good-- I think that your design is pretty good-- your HotelBooker manages the reservations, rooms, reserved blocks, and unreserved blocks. The complex date overlap logic lives in the DateRange class, which I think really paid off.

Overall, my biggest comment is that I truly feel that the tests were thorough, clean, and readable. I in fact have very little comments on the tests because I think they're great-- wish I had more to comment on, but I really think you did a great job on the tests!

That being said, I'm adding some comments on specific pieces of code. The main themes on these comments are:

Suggestions for minor refactorings to clean up code style
Weirdly enough, I think you're pretty inconsistent with checking if an array is empty, so I'm calling that out a little specifically??

In conclusion: generally good job! I think I could see your code being a tiny bit cleaner, but the DateRange decision and the tests you wrote are really great and overwhelmingly positive.

tildeee · 2018-09-21T06:12:24Z

lib/hotel_booker.rb

+  class HotelBooker
+    attr_accessor :rooms, :reservations, :unreserved_block, :reserved_block
+    NUM_ROOMS = 20
+    BLOCK_MAX = 5


YES CONSTANTS

tildeee · 2018-09-21T06:13:08Z

lib/hotel_booker.rb

+      @rooms = []
+
+      NUM_ROOMS.times do |i|
+        @rooms << Hotel::Room.new(i+1)


Ruby will be looking for names locally first, prioritizing local names... so, since they're all in the same module, there's no need to namespace Hotel::Room... Room should just work! Same with the rest of the namespacing in the code.

tildeee · 2018-09-21T06:13:32Z

lib/hotel_booker.rb

+      check_in = Date.parse(check_in)
+      check_out = Date.parse(check_out)
+      date_range = range(check_in, check_out)
+      reservation = Hotel::Reservation.new(id, date_range)


It might give you less "overusing the word range"-anxiety to in-line this:

reservation = Hotel::Reservation.new(id, range(check_in, check_out))

tildeee · 2018-09-21T06:13:51Z

lib/hotel_booker.rb

+      available = unreserved_rooms(check_in, check_out)
+      if available == []
+        raise StandardError, "There are no more available rooms for this date range!"
+      end


It probably reads better to write this as if available.empty?, or maybe inline it somehow ;)

tildeee · 2018-09-21T06:14:08Z

lib/hotel_booker.rb

+    end
+
+
+    def make_block(rooms: nil, discount: nil, check_in: nil, check_out: nil)


This keyword args syntax makes these parameters optional: is that what you want?

tildeee · 2018-09-21T06:14:39Z

lib/hotel_booker.rb

+    def make_block_reservation(id)
+      if @unreserved_block.length ==  0
+        raise StandardError, "There are no available Block reservations."
+      end


I think this is another block of code that checks if an array is empty in a different way? If so -- check my comment above!

tildeee · 2018-09-21T06:15:03Z

lib/hotel_booker.rb

+      unreserved_block_rooms = []
+      @unreserved_block.each do |reservation|
+        unreserved_block_rooms << reservation.room
+      end


Could possibly be a good chance to use map

tildeee · 2018-09-21T06:15:18Z

lib/hotel_booker.rb

+        if reservation_dates.include?(date)
+          matching_reservations << reservation
+        end
+      end


Instead of using .each, might be good opportunity to use select

tildeee · 2018-09-21T06:15:32Z

refactors.txt

@@ -0,0 +1,7 @@
+1. I...lowkey cheated and made a collection of Reservations instead of a collection of Rooms. I would rewrite this part by making a subclass under Room called BookedRoom?


Not really sure what you mean by this tbh!

tildeee · 2018-09-21T06:15:41Z

refactors.txt

@@ -0,0 +1,7 @@
+1. I...lowkey cheated and made a collection of Reservations instead of a collection of Rooms. I would rewrite this part by making a subclass under Room called BookedRoom?
+
+2. HotelBooker has a lot of WET code because I have to loop through several data structures (@unreserved_block, @reserved_block, @reservations)!! If I had focused the Wave 3 blocks of rooms on the ROOMS instead of reservations, probably wouldn't have created this mess ¯\_(ツ)_/¯


tildeee · 2018-09-21T06:15:53Z

refactors.txt

+
+2. HotelBooker has a lot of WET code because I have to loop through several data structures (@unreserved_block, @reserved_block, @reservations)!! If I had focused the Wave 3 blocks of rooms on the ROOMS instead of reservations, probably wouldn't have created this mess ¯\_(ツ)_/¯
+
+3. there are certain check_in and check_out arguments that are strings (as in Hotel::HotelBooker.new.make_reservation) and some that are an instance of Date (as in Hotel::HotelBooker.new.unreserved_rooms). Other arguments pass in a Hotel::DateRange (such as Hotel::Reservations). note to future goeun: make this less terrible


Would absolutely agree with this

tildeee · 2018-09-21T06:16:12Z

spec/date_range_spec.rb

+      expect(@date_range2.overlaps?(@date_range4)).must_equal false
+    end
+  end
+end


I love this DateRange class because it isolated the overlapping date logic to this one class, which is complex and tested in isolation... Beautiful :* truly inspiring :*

goeunpark added 23 commits September 4, 2018 17:00

create spec and lib files for hotel booker, calendar, reservation, an…

a79e8c4

…d room

HotelBooker initates 20 rooms, can list all rooms, can reserve a room…

73609dc

…, and can get total_cost of reservation

renamed booked_dates to date_range, started spec and method calls

44059e3

overlap? method in date_range returns boolean if two date ranges overlap

d0ba0bf

overlaps? method in date_range returns boolean

d4e5b6d

test for raising standard error for invalid date ranges is red

62b0679

raises standard error for invalid date ranges: green

b054817

raises ArgumentError if dates argument are not Dates

aaedaaf

change arguments in reservation to date_range, not check_in, check_ou…

fe5d223

…t; test for find_reservations(date) in hotel_booker is red

find_reservation in hotel_booker passes tests

bfbb588

split up tests for overlaps? in date_range

fe6c4f2

unreserved_rooms method in hotel_booker failes test

caaf57b

initial pass for unreserved_rooms method in hotel_booker

c0cee89

clean up Room and room_spec files

b0fc136

cleaned up Reservation and Reservation_spec files

a431409

updated unreserved_rooms method testing in hotel_booker_spec

4859efe

start tests for create_block method in hotel_booker

e8de7fe

hotel_booker can create a block of rooms

4f89d83

reservation dates match the date range of the block test passes

abeee86

start make_block_reservation method

5a90808

can book a block reservation

c4d7e68

create unreserved_block_rooms method and passing test

a963903

add a refactors file

e781d16

tildeee reviewed Sep 21, 2018

View reviewed changes

goeunpark added 7 commits September 30, 2018 15:32

remove unnecessary namespacing

eca8b38

refactor checking available rooms into its own method

1be5ba3

start design-activity responses

8278260

add bolded questions

108afe7

finish design-activity questions

df6bd13

refactor unreserved_block_rooms method

821a8db

refactor / design in changing cost of room when creating blocks

96f44ed

		end


		def make_block(rooms: nil, discount: nil, check_in: nil, check_out: nil)

		@@ -0,0 +1,7 @@
		1. I...lowkey cheated and made a collection of Reservations instead of a collection of Rooms. I would rewrite this part by making a subclass under Room called BookedRoom?

		@@ -0,0 +1,7 @@
		1. I...lowkey cheated and made a collection of Reservations instead of a collection of Rooms. I would rewrite this part by making a subclass under Room called BookedRoom?

		2. HotelBooker has a lot of WET code because I have to loop through several data structures (@unreserved_block, @reserved_block, @reservations)!! If I had focused the Wave 3 blocks of rooms on the ROOMS instead of reservations, probably wouldn't have created this mess ¯\_(ツ)_/¯


		2. HotelBooker has a lot of WET code because I have to loop through several data structures (@unreserved_block, @reserved_block, @reservations)!! If I had focused the Wave 3 blocks of rooms on the ROOMS instead of reservations, probably wouldn't have created this mess ¯\_(ツ)_/¯

		3. there are certain check_in and check_out arguments that are strings (as in Hotel::HotelBooker.new.make_reservation) and some that are an instance of Date (as in Hotel::HotelBooker.new.unreserved_rooms). Other arguments pass in a Hotel::DateRange (such as Hotel::Reservations). note to future goeun: make this less terrible

Conversation

goeunpark commented Sep 10, 2018

Hotel

Comprehension Questions

Uh oh!

tildeee commented Sep 21, 2018

Hotel

What We're Looking For

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants